(TWILL-254) Update to use ContainerId.fromString #65
Conversation
pom.xml
Outdated
| @@ -171,7 +171,7 @@ | |||
| <junit.version>4.11</junit.version> | |||
| <jopt-simple.version>3.2</jopt-simple.version> | |||
| <commons-compress.version>1.5</commons-compress.version> | |||
| <hadoop.version>[2.0.2-alpha,2.3.0]</hadoop.version> | |||
| <hadoop.version>2.7.2</hadoop.version> | |||
| <hadoop20.output.dir>target/hadoop20-classes</hadoop20.output.dir> | |||
There was a problem hiding this comment.
is there any reason to keep this property at all? seems better to just have it defined in each profile as necessary.
pom.xml
Outdated
| <properties> | ||
| <hadoop.version>2.5.1</hadoop.version> | ||
| <hadoop.version>2.6.5</hadoop.version> |
There was a problem hiding this comment.
why are you eliminating the hadoop-2.5 profile?
There was a problem hiding this comment.
I didn't see any code specific to 2.5 that 2.3 did not provide?
There was a problem hiding this comment.
The profile for different versions of Hadoop is both for version specific code as well as for running unit-tests. It is better to keep it and add the hadoop-2.6
There was a problem hiding this comment.
@chtyim You are asking me to leave the 2.5 profile alone and add the 2.6 profile?
| @@ -26,7 +26,7 @@ | |||
| /** | |||
| * Wrapper class for AMRMClient for Hadoop version 2.2 or greater. | |||
| */ | |||
| public final class Hadoop22YarnAMClient extends Hadoop21YarnAMClient { | |||
| public class Hadoop22YarnAMClient extends Hadoop21YarnAMClient { | |||
|
|
|||
There was a problem hiding this comment.
If marked final, I can not extend it for Hadoop26YarnAMClient?
There was a problem hiding this comment.
Ah, okay then. sorry, I missed that.
| @@ -71,6 +73,11 @@ public Hadoop20YarnAMClient(Configuration conf) { | |||
| this.nmClient = new Hadoop20YarnNMClient(YarnRPC.create(conf), conf); | |||
| } | |||
|
|
|||
| @Override | |||
| private ContainerId containerIdLookup(String containerIdStr) { | |||
| return (ConverterUtils.toContainerId(containerIdStr)); | |||
There was a problem hiding this comment.
this should be protected not private
cbaenziger
force-pushed
the
master
branch
2 times, most recently
from
80c25ee
to
9f5501d
Compare
|
I added a few omissions and corrected the parent |
|
Thank you for the super fast code review @serranom! I think all comments have been addressed and the code should be complete now. Please let me know if you see anything else necessary. It is great working with Twill! Hopefully I can provide some other ideas as I work with it. |
|
The change looks mostly ok to me, except for the change in the pom.xml profile. |
|
Hi @chtyim, thank you for the review. Could you please expand on what you see concerning in the |
|
Changes LGTM. |
This closes #65 on Github. Signed-off-by: Terence Yim <chtyim@apache.org>
This moves away from the deprecated
ConverterUitls.toContainerIdand updates the build to use Hadoop 2.7.2 to avoid folks grabbing master and having a failure trying to run on clusters with code newer than November 18th, 2014 (release of Hadoop 2.6.0).